Review — Click → Jakarta Servlets + FreeMarker migration

I focused on the real review surface (the new servlet infrastructure, the ported wizard pages and templates), checking behavior against the originals on master rather than the ~36k lines of deleted vendored Click fork.

Verdict: no blocking bugs or regressions found. This is a careful, behavior-preserving migration with good test coverage. A few cosmetic leftovers and hardening opportunities below; none are merge blockers.

What holds up well

• Reflection dispatch is safely gated. ConfiguratorServlet.findAction only resolves methods annotated with @ConfiguratorAction, so a request cannot invoke an arbitrary public method. Unknown actionLink → 404, unregistered path → 404.
• FreeMarker's null-strictness is handled systematically. Velocity rendered a missing $foo as a literal; FreeMarker ${foo} with RETHROW_HANDLER throws. Pages either always populate values (getAttribute(..., default)) or templates guard with ${x!""} / <#if x??>. The trickiest page, step3.ftl (${type}, <#if store.password??>), is covered because both come from LDAPStoreWizardPage.onInit() (type="config", and store is never null via ensureConfig()). This is backstopped by ConfiguratorServletRenderSmokeTest, which renders all 10 pages and asserts no exception.
• Security posture is faithfully ported. ProtectedSetupPage.onSecurityCheck() reproduces the old ProtectedPage re-entry guard (AMSetupServlet.isConfigured()); Options deliberately extends SetupPage without a guard, matching the old Options extends TemplatedPage.
• Routing / downstream registration are clean. web.xml swaps click-servlet → configurator-servlet on *.htm; Upgrade (in openam-upgrade) self-registers via ServiceLoader/META-INF/services (same idiom as SetupListener), avoiding a Maven cycle. No dangling references to the removed openidentityplatform.openam.click package remain in live code.
• Prior review rounds already fixed real issues (NPE in checkPasswords, JSON escaping via JSONObject, TemplateExceptionHandler).

Minor cleanup (worth doing in this PR)

1. Stale WAR manifests. fam-console.list#L1239 and fam-noconsole.list#L1334 still list WEB-INF/click.xml and WEB-INF/classes/click-page.properties, which this PR deletes. The build doesn't break (only the legacy Ant src/main/previous_scripts/build.xml reads these), but the lists are now inaccurate.
2. Stale docs. chap-endpoints.adoc / chap-endpoints.xml still describe the click-servlet endpoint.
3. copyPublicFields precedence. ConfiguratorServlet.copyPublicFields runs after the model is seeded from getModel(), so a public field silently overrides an addModel entry of the same name. Harmless today (values coincide), but a latent footgun — a comment would help.

Observations — pre-existing, not introduced by this PR (no action required here)

4. No FreeMarker HTML auto-escaping. Form values (serverURL, configStoreHost, rootSuffix, LDAP hosts, step3.ftl password into value=, and the step7 summary) render unescaped — exactly as the old Velocity $foo did. Not a regression, and these pages run pre-configuration. But since this is a rewrite it's a natural place to consider HTMLOutputFormat. Caveat: it can't be a blind flip — some templates intentionally inject HTML built in Java (e.g. Step2's initialCheck), which would need ?no_esc. Reasonable to defer to a follow-up.
5. validateInput sets an arbitrary session attribute from request key/value — a faithful port of the old AjaxPage. See SetupPage.validateInput. Low risk within the setup wizard's trust model.
6. Dead pushConfig / testNewInstanceUrl action paths. wizard.ftl#L157 still calls actionLink=pushConfig, but no handler exists → 404. The old Wizard.java only had ActionLink fields pointing at methods that were never defined (only createConfig() had a body), so this path was already broken (Click would have errored). Carried over as-is; a 404 now instead of a 500.
7. step7 summary embedded check. step7.ftl#L24 <#if embedded??> is never true because Step7.java#L57 adds isEmbedded, not embedded — but the old step7.htm had the identical #if($embedded) vs add("isEmbedded",...) mismatch. Faithful port of a latent bug.

Review: Migrate configurator/upgrade wizard from Apache Click to Jakarta Servlets + FreeMarker

Overview

The PR replaces the abandoned, vendored Apache Click fork (~36k LOC) that powered the 13-page configurator/upgrade wizard with a small purpose-built stack: ConfiguratorServlet (explicit path→page registry + @ConfiguratorAction reflection dispatch), a SetupPage/ProtectedSetupPage base-class pair ported from AjaxPage/ProtectedPage, a Jakarta-compatible FreeMarker TemplateLoader, and .ftl ports of every wizard template. Downstream modules (openam-upgrade) self-register pages via ServiceLoader, avoiding a Maven cycle. URLs, form field names, and actionLink parameter names are preserved byte-for-byte.

Verdict: high-quality migration — approve after fixing the copyright-header issues and deciding on fam-noconsole.list. Every migrated page class was cross-checked against its merge-base original, every .ftl against its .htm, all 28 ?no_esc sites, all actionLink names against @ConfiguratorAction methods, and the FreeMarker missing-variable surface — no correctness regressions found in reachable code. The code is unusually well-commented about why parity decisions were made.

Issues to fix before merge

1. Copyright/provenance — dropped upstream notices (CDDL compliance).

• SetupPage.java is presented as a new file with only Copyright 2026 3A Systems LLC., but it ports substantial verbatim logic (validateInput, checkPasswords, setLocale, getBaseDir, getLocalizedString, …) from AjaxPage.java, whose header read Copyright (c) 2007 Sun Microsystems Inc. + Portions Copyrighted 2011-2016 ForgeRock AS. under an explicit "DO NOT ALTER OR REMOVE COPYRIGHT NOTICES" banner. The Sun/ForgeRock lines must be carried over.
• ProtectedSetupPage.java is a rename of ProtectedPage.java (git similarity 53%) that replaced Copyright 2014 ForgeRock AS. with the 3A Systems line instead of adding a Portions line.
• Minor same-family items: step7.ftl is a new file with no license header at all; upgrade.ftl was substantially modified but gained no 3A Systems Portions line; DefaultSummary.java header year not extended (2025 → should be 2025-2026).

2. fam-noconsole.list is now internally inconsistent (medium, with a fossil caveat). It still lists the deleted ./config/wizard/step*.htm, ./config/options.htm, ./config/defaultSummary.htm, adds no WEB-INF/templates/config/*.ftl entries and no freemarker jar — while its web.xml was updated to map *.htm → ConfiguratorServlet. A createwar noconsole WAR would 500 on every configurator page (TemplateNotFound). Caveat: the whole list is OpenSSO-era fossil content (opensso.jar, velocity-1.7.jar), so the pipeline is likely already dead — either make the list consistent or leave the file untouched as a fossil, but don't half-update it.

3. Dangling references. Shipped javadoc/comments point at docs/migration/click-to-freemarker/03-migration-plan.md / 04-implementation-notes.md (e.g. ConfiguratorServlet.java:55, Step3.java:163, Step6.java:63, several tests) — no docs/ directory exists on the branch. Also legal/THIRDPARTYREADME.txt still attributes the removed click-nodeps-2.3.0.jar/click-extras-2.3.0.jar.

Security — net improvement

• Closes real stored-XSS sinks. HTMLOutputFormat auto-escaping replaces Click/Velocity's raw output; merge-base templates echoed session-writable values (configStoreHost, rootSuffix, encryptionKey, serverURL, store.password, step7 summary) unescaped into attributes. All 28 ?no_esc sites were audited: every one is server-generated markup or a bundle string that legitimately contains HTML — none carry request-controlled data. Localized strings in JS contexts correctly use ?js_string?no_esc. No injection path found.
• validateInput is now allowlisted to the 8 session keys the wizard JS actually sends. The old AjaxPage let a hand-crafted ?actionLink=validateInput&key=...&value=... write any session attribute — including ADMIN_PWD, bypassing checkPasswords validation. Verified complete against every template caller; no shipped functionality blocked.
• @ConfiguratorAction narrows the dispatch surface — only annotated methods are reachable, vs Click's public-field ActionLinks.
• Nit: overriding service() means every HTTP method (PUT, DELETE, TRACE…) runs the full lifecycle including action dispatch; old ClickServlet exposed only GET/POST. Consider rejecting non-GET/POST/HEAD.

Correctness / parity (verified, not just sampled)

• Every old ProtectedPage subclass now extends ProtectedSetupPage; Options and Upgrade correctly stay unprotected (required for the upgrade path on configured installs). Guard behavior (empty 200) is identical.
• All action bodies are byte-equivalent apart from the setPath(null) → skipRender() translation, which is semantically identical; lifecycle order (onSecurityCheck → onInit → action/onGet) matches Click, including Wizard's eager port-scan initializers correctly moved to onInit().
• FreeMarker RETHROW_HANDLER + missing-variable audit: every unconditional ${var} is unconditionally populated; every conditional value is ??/!""-guarded (including the ported selectLDAPv3opends ODSEE quirk) — no render-time 500 risk found.
• Two deliberate, tested behavior deltas worth noting in the PR description: step7 now actually renders the embedded-store Admin/JMX port rows (the old template guarded on a never-set $embedded key — a fix, covered both directions by the render smoke test), and the dead pushConfig dialog (whose backing methods never existed at merge base) was dropped from wizard.ftl.
• Only reachable-in-theory regression: step4.htm?actionLink=clearStore (via wizard.ftl's disableCustomConfig, which has zero callers) now 404s instead of harmlessly rendering. Dead code today.

Build & tests

• org.freemarker:freemarker is version-managed in the root pom (2.3.31, pre-existing) and already shipped in the WAR via openam-server-only; no pom still references Click; click.version and the javadoc package excludes are cleaned up.
• Tests fit the modules' conventions (TestNG + Mockito + AssertJ, all versions already managed; no new deps). Coverage is substantive, not mock-theater: ConfiguratorServletTest drives real service() against the real registry (including the allowlist refusals and 404 paths); ConfiguratorServletRenderSmokeTest renders the actual .ftl files and asserts the XSS-escaping, ?no_esc, ?js_string, and step7 toggle behavior; ConfiguratorServletUpgradeRoutingTest is the only genuine end-to-end proof of the ServiceLoader wiring. Weak spots are honest and documented (OptionsTest/DefaultSummaryTest near-tautological due to AMSetupServlet coupling).
• Test nits: Step7Test:100 comment contradicts the shipped template (the port did fix the $embedded mismatch); ConfiguratorServletUpgradeRoutingTest:77 still mocks a click-servlet named dispatcher nothing uses.

Suggested follow-ups (non-blocking)

• Fix the stale SetupPage comment claiming DefaultSummary is "not yet migrated".
• checkPasswords with a missing type param now silently stores the admin password where old code NPE'd — consider rejecting instead (hand-crafted requests only).
• The PR's "manual wizard walkthrough recommended pre-merge" note stands: fresh-install + upgrade flow, since this is the pre-configuration bootstrap path and the render smoke test can't cover the live AMSetupServlet.processRequest handoff.